-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Annotate CPU and goroutine profiles #4204
Conversation
37227b4
to
9aa3a6c
Compare
Lots of tests failures that should be adressed. |
Yep, PR is still marked as draft as I haven't looked at those failures yet, but I've gone back to the consumer defaults for now. |
9aa3a6c
to
b5ed7d9
Compare
server/opts.go
Outdated
@@ -986,6 +987,8 @@ func (o *Options) processConfigFileLine(k string, v interface{}, errors *[]error | |||
o.PortsFileDir = v.(string) | |||
case "prof_port": | |||
o.ProfPort = int(v.(int64)) | |||
case "prof_tags": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed, if you turn on profiling we will add in the tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it's OK to always tag? It will mean for the first time that the profiles contain potentially sensitive information (account/stream/consumer names) with no way to toggle, whereas before they did not contain anything that really uniquely identifies a specific environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see an argument for both. If someone was sharing with us let's say would want all the information so that we could tie together any NATS cli triage, e.g. nats stream info
with any profiling they may share.
b5ed7d9
to
387a118
Compare
prof_tags
for annotating CPU and goroutine profiles
This was modified to just include the tags in all cases and removed the configuration option. |
server/jetstream_cluster.go
Outdated
js.srv.startGoRoutine(js.monitorCluster) | ||
js.srv.startGoRoutine( | ||
js.monitorCluster, | ||
map[string]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yea this seems worse. Maybe back to strings but concat the key and value?
js.srv.startGoRoutine( js.monitorCluster, "type: metaleader", ..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only thing with this approach is that it might end up ugly for other reasons, i.e. fmt.Sprintf
littered everywhere, but I'll happily go with that if that is what you prefer:
js.srv.startGoRoutine( js.monitorCluster, "type:metaleader", fmt.Sprintf("stream:%s", streamName), ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, true.
Signed-off-by: Neil Twigg <neil@nats.io>
387a118
to
d2615b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This allows the CPU and goroutine profiles to be annotated with information that allows us to break down load based on accounts, streams and consumers. We could probably add more labels elsewhere for other purposes too. It makes it easier to spot whether there are certain assets that are responsible for heavy CPU usage, i.e. snapshotting certain stream states.
Signed-off-by: Neil Twigg neil@nats.io